From: Stephen Becker IV Date: Thu, 12 May 2016 01:34:15 +0000 (-0700) Subject: Network retry issue 1602 X-Git-Tag: archive/raspbian/0.35.0-2+rpi1~3^2^2^2^2^2^2^2~22^2~14^2~19^2 X-Git-Url: https://dgit.raspbian.org/%22http://www.example.com/cgi/success//%22http:/www.example.com/cgi/success/?a=commitdiff_plain;h=7b03532b4f58eea1ccdb397a1b8500fe797df198;p=cargo.git Network retry issue 1602 Dearest Reviewer, This branch resolves #1602 which relates to retrying network issues automatically. There is a new utility helper for retrying any network call. There is a new config called net.retry value in the .cargo.config file. The default value is 2. The documentation has also been updated to reflect the new value. Thanks Becker --- diff --git a/Cargo.lock b/Cargo.lock index 141402459..fb69826ce 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -7,6 +7,7 @@ dependencies = [ "crates-io 0.2.0", "crossbeam 0.2.8 (registry+https://github.com/rust-lang/crates.io-index)", "curl 0.2.19 (registry+https://github.com/rust-lang/crates.io-index)", + "curl-sys 0.1.34 (registry+https://github.com/rust-lang/crates.io-index)", "docopt 0.6.78 (registry+https://github.com/rust-lang/crates.io-index)", "env_logger 0.3.2 (registry+https://github.com/rust-lang/crates.io-index)", "filetime 0.1.10 (registry+https://github.com/rust-lang/crates.io-index)", diff --git a/Cargo.toml b/Cargo.toml index a5d673869..c3a4c48c2 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -21,6 +21,7 @@ advapi32-sys = "0.1" crates-io = { path = "src/crates-io", version = "0.2" } crossbeam = "0.2" curl = "0.2" +curl-sys = "0.1" docopt = "0.6" env_logger = "0.3" filetime = "0.1" diff --git a/src/cargo/lib.rs b/src/cargo/lib.rs index b3c9c9bbe..c1012bc40 100644 --- a/src/cargo/lib.rs +++ b/src/cargo/lib.rs @@ -6,6 +6,7 @@ extern crate crates_io as registry; extern crate crossbeam; extern crate curl; +extern crate curl_sys; extern crate docopt; extern crate filetime; extern crate flate2; diff --git a/src/cargo/sources/git/source.rs b/src/cargo/sources/git/source.rs index b47dda80a..5c753b546 100644 --- a/src/cargo/sources/git/source.rs +++ b/src/cargo/sources/git/source.rs @@ -162,7 +162,8 @@ impl<'cfg> Source for GitSource<'cfg> { format!("git repository `{}`", self.remote.url()))); trace!("updating git source `{:?}`", self.remote); - let repo = try!(self.remote.checkout(&db_path)); + + let repo = try!(self.remote.checkout(&db_path, &self.config)); let rev = try!(repo.rev_for(&self.reference)); (repo, rev) } else { @@ -172,7 +173,7 @@ impl<'cfg> Source for GitSource<'cfg> { // Copy the database to the checkout location. After this we could drop // the lock on the database as we no longer needed it, but we leave it // in scope so the destructors here won't tamper with too much. - try!(repo.copy_to(actual_rev.clone(), &checkout_path)); + try!(repo.copy_to(actual_rev.clone(), &checkout_path, &self.config)); let source_id = self.source_id.with_precise(Some(actual_rev.to_string())); let path_source = PathSource::new_recursive(&checkout_path, diff --git a/src/cargo/sources/git/utils.rs b/src/cargo/sources/git/utils.rs index d4c540e36..068dc3cf0 100644 --- a/src/cargo/sources/git/utils.rs +++ b/src/cargo/sources/git/utils.rs @@ -8,7 +8,7 @@ use url::Url; use git2::{self, ObjectType}; use core::GitReference; -use util::{CargoResult, ChainError, human, ToUrl, internal}; +use util::{CargoResult, ChainError, human, ToUrl, internal, Config, network}; #[derive(PartialEq, Clone, Debug)] pub struct GitRevision(git2::Oid); @@ -109,16 +109,16 @@ impl GitRemote { db.rev_for(reference) } - pub fn checkout(&self, into: &Path) -> CargoResult { + pub fn checkout(&self, into: &Path, cargo_config: &Config) -> CargoResult { let repo = match git2::Repository::open(into) { Ok(repo) => { - try!(self.fetch_into(&repo).chain_error(|| { + try!(self.fetch_into(&repo, &cargo_config).chain_error(|| { human(format!("failed to fetch into {}", into.display())) })); repo } Err(..) => { - try!(self.clone_into(into).chain_error(|| { + try!(self.clone_into(into, &cargo_config).chain_error(|| { human(format!("failed to clone into: {}", into.display())) })) } @@ -140,21 +140,21 @@ impl GitRemote { }) } - fn fetch_into(&self, dst: &git2::Repository) -> CargoResult<()> { + fn fetch_into(&self, dst: &git2::Repository, cargo_config: &Config) -> CargoResult<()> { // Create a local anonymous remote in the repository to fetch the url let url = self.url.to_string(); let refspec = "refs/heads/*:refs/heads/*"; - fetch(dst, &url, refspec) + fetch(dst, &url, refspec, &cargo_config) } - fn clone_into(&self, dst: &Path) -> CargoResult { + fn clone_into(&self, dst: &Path, cargo_config: &Config) -> CargoResult { let url = self.url.to_string(); if fs::metadata(&dst).is_ok() { try!(fs::remove_dir_all(dst)); } try!(fs::create_dir_all(dst)); let repo = try!(git2::Repository::init_bare(dst)); - try!(fetch(&repo, &url, "refs/heads/*:refs/heads/*")); + try!(fetch(&repo, &url, "refs/heads/*:refs/heads/*", &cargo_config)); Ok(repo) } } @@ -164,13 +164,13 @@ impl GitDatabase { &self.path } - pub fn copy_to(&self, rev: GitRevision, dest: &Path) + pub fn copy_to(&self, rev: GitRevision, dest: &Path, cargo_config: &Config) -> CargoResult { let checkout = match git2::Repository::open(dest) { Ok(repo) => { let checkout = GitCheckout::new(dest, self, rev, repo); if !checkout.is_fresh() { - try!(checkout.fetch()); + try!(checkout.fetch(&cargo_config)); try!(checkout.reset()); assert!(checkout.is_fresh()); } @@ -178,7 +178,7 @@ impl GitDatabase { } Err(..) => try!(GitCheckout::clone_into(dest, self, rev)), }; - try!(checkout.update_submodules().chain_error(|| { + try!(checkout.update_submodules(&cargo_config).chain_error(|| { internal("failed to update submodules") })); Ok(checkout) @@ -276,12 +276,12 @@ impl<'a> GitCheckout<'a> { } } - fn fetch(&self) -> CargoResult<()> { + fn fetch(&self, cargo_config: &Config) -> CargoResult<()> { info!("fetch {}", self.repo.path().display()); let url = try!(self.database.path.to_url().map_err(human)); let url = url.to_string(); let refspec = "refs/heads/*:refs/heads/*"; - try!(fetch(&self.repo, &url, refspec)); + try!(fetch(&self.repo, &url, refspec, &cargo_config)); Ok(()) } @@ -303,10 +303,10 @@ impl<'a> GitCheckout<'a> { Ok(()) } - fn update_submodules(&self) -> CargoResult<()> { - return update_submodules(&self.repo); + fn update_submodules(&self, cargo_config: &Config) -> CargoResult<()> { + return update_submodules(&self.repo, &cargo_config); - fn update_submodules(repo: &git2::Repository) -> CargoResult<()> { + fn update_submodules(repo: &git2::Repository, cargo_config: &Config) -> CargoResult<()> { info!("update submodules for: {:?}", repo.workdir().unwrap()); for mut child in try!(repo.submodules()).into_iter() { @@ -346,14 +346,14 @@ impl<'a> GitCheckout<'a> { // Fetch data from origin and reset to the head commit let refspec = "refs/heads/*:refs/heads/*"; - try!(fetch(&repo, url, refspec).chain_error(|| { + try!(fetch(&repo, url, refspec, &cargo_config).chain_error(|| { internal(format!("failed to fetch submodule `{}` from {}", child.name().unwrap_or(""), url)) })); let obj = try!(repo.find_object(head, None)); try!(repo.reset(&obj, git2::ResetType::Hard, None)); - try!(update_submodules(&repo)); + try!(update_submodules(&repo, &cargo_config)); } Ok(()) } @@ -389,7 +389,7 @@ impl<'a> GitCheckout<'a> { /// attempted and we don't try the same ones again. fn with_authentication(url: &str, cfg: &git2::Config, mut f: F) -> CargoResult - where F: FnMut(&mut git2::Credentials) -> Result + where F: FnMut(&mut git2::Credentials) -> CargoResult { let mut cred_helper = git2::CredentialHelper::new(url); cred_helper.config(cfg); @@ -555,7 +555,7 @@ fn with_authentication(url: &str, cfg: &git2::Config, mut f: F) } pub fn fetch(repo: &git2::Repository, url: &str, - refspec: &str) -> CargoResult<()> { + refspec: &str, cargo_config: &Config) -> CargoResult<()> { // Create a local anonymous remote in the repository to fetch the url with_authentication(url, &try!(repo.config()), |f| { @@ -565,7 +565,10 @@ pub fn fetch(repo: &git2::Repository, url: &str, let mut opts = git2::FetchOptions::new(); opts.remote_callbacks(cb) .download_tags(git2::AutotagOption::All); - try!(remote.fetch(&[refspec], Some(&mut opts), None)); + + try!(network::with_retry(cargo_config, ||{ + remote.fetch(&[refspec], Some(&mut opts), None) + })); Ok(()) }) } diff --git a/src/cargo/sources/registry.rs b/src/cargo/sources/registry.rs index c1695da6b..614d6540a 100644 --- a/src/cargo/sources/registry.rs +++ b/src/cargo/sources/registry.rs @@ -483,7 +483,8 @@ impl<'cfg> RegistrySource<'cfg> { // git fetch origin let url = self.source_id.url().to_string(); let refspec = "refs/heads/*:refs/remotes/origin/*"; - try!(git::fetch(&repo, &url, refspec).chain_error(|| { + + try!(git::fetch(&repo, &url, refspec, &self.config).chain_error(|| { internal(format!("failed to fetch `{}`", url)) })); diff --git a/src/cargo/util/config.rs b/src/cargo/util/config.rs index 9f41c56f5..162f83bd2 100644 --- a/src/cargo/util/config.rs +++ b/src/cargo/util/config.rs @@ -265,6 +265,21 @@ impl Config { } } + pub fn net_retry(&self) -> CargoResult { + match try!(self.get_i64("net.retry")) { + Some(v) => { + let value = v.val; + if value < 0 { + bail!("net.retry must be positive, but found {} in {}", + v.val, v.definition) + } else { + Ok(value) + } + } + None => Ok(2), + } + } + pub fn expected(&self, ty: &str, key: &str, val: CV) -> CargoResult { val.expected(ty).map_err(|e| { human(format!("invalid configuration for key `{}`\n{}", key, e)) diff --git a/src/cargo/util/errors.rs b/src/cargo/util/errors.rs index bb213b25e..b7c03c210 100644 --- a/src/cargo/util/errors.rs +++ b/src/cargo/util/errors.rs @@ -8,6 +8,7 @@ use std::str; use std::string; use curl; +use curl_sys; use git2; use rustc_serialize::json; use semver; @@ -285,6 +286,36 @@ impl From> for CliError { } } +// ============================================================================= +// NetworkError trait + +pub trait NetworkError: CargoError { + fn maybe_spurious(&self) -> bool; +} + +impl NetworkError for git2::Error { + fn maybe_spurious(&self) -> bool { + match self.class() { + git2::ErrorClass::Net | + git2::ErrorClass::Os => true, + _ => false + } + } +} +impl NetworkError for curl::ErrCode { + fn maybe_spurious(&self) -> bool { + match self.code() { + curl_sys::CURLcode::CURLE_COULDNT_CONNECT | + curl_sys::CURLcode::CURLE_COULDNT_RESOLVE_PROXY | + curl_sys::CURLcode::CURLE_COULDNT_RESOLVE_HOST | + curl_sys::CURLcode::CURLE_OPERATION_TIMEDOUT | + curl_sys::CURLcode::CURLE_RECV_ERROR + => true, + _ => false + } + } +} + // ============================================================================= // various impls diff --git a/src/cargo/util/mod.rs b/src/cargo/util/mod.rs index 798c0f9fc..d638691d7 100644 --- a/src/cargo/util/mod.rs +++ b/src/cargo/util/mod.rs @@ -32,6 +32,7 @@ pub mod to_url; pub mod toml; pub mod lev_distance; pub mod job; +pub mod network; mod cfg; mod dependency_queue; mod rustc; diff --git a/src/cargo/util/network.rs b/src/cargo/util/network.rs new file mode 100644 index 000000000..a53d04d28 --- /dev/null +++ b/src/cargo/util/network.rs @@ -0,0 +1,86 @@ +use util::{CargoResult, Config, errors}; + +/// Wrapper method for network call retry logic. +/// +/// Retry counts provided by Config object 'net.retry'. Config shell outputs +/// a warning on per retry. +/// +/// Closure must return a CargoResult. +/// +/// Example: +/// use util::network; +/// cargo_result = network.with_retry(&config, || something.download()); +pub fn with_retry(config: &Config, mut callback: F) -> CargoResult + where F: FnMut() -> Result, + E: errors::NetworkError +{ + let mut remaining = try!(config.net_retry()); + loop { + match callback() { + Ok(ret) => return Ok(ret), + Err(ref e) if e.maybe_spurious() && remaining > 0 => { + let msg = format!("spurious network error ({} tries \ + remaining): {}", remaining, e); + try!(config.shell().warn(msg)); + remaining -= 1; + } + Err(e) => return Err(Box::new(e)), + } + } +} +#[test] +fn with_retry_repeats_the_call_then_works() { + + use std::error::Error; + use util::human; + use std::fmt; + + #[derive(Debug)] + struct NetworkRetryError { + error: Box, + } + + impl Error for NetworkRetryError { + fn description(&self) -> &str { + self.error.description() + } + fn cause(&self) -> Option<&Error> { + self.error.cause() + } + } + + impl NetworkRetryError { + fn new(error: &str) -> NetworkRetryError { + let error = human(error.to_string()); + NetworkRetryError { + error: error, + } + } + } + + impl fmt::Display for NetworkRetryError { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + fmt::Display::fmt(&self.error, f) + } + } + + impl errors::CargoError for NetworkRetryError { + fn is_human(&self) -> bool { + false + } + } + + impl errors::NetworkError for NetworkRetryError { + fn maybe_spurious(&self) -> bool { + true + } + } + + let error1 = NetworkRetryError::new("one"); + let error2 = NetworkRetryError::new("two"); + let mut results: Vec> = vec![Ok(()), + Err(error1), Err(error2)]; + let config = Config::default().unwrap(); + let result = with_retry(&config, || results.pop().unwrap()); + assert_eq!(result.unwrap(), ()) +} diff --git a/src/doc/config.md b/src/doc/config.md index 2db724785..25eb25bc6 100644 --- a/src/doc/config.md +++ b/src/doc/config.md @@ -89,6 +89,10 @@ rustflags = ["..", ".."] # custom flags to pass to all compiler invocations [term] verbose = false # whether cargo provides verbose output color = 'auto' # whether cargo colorizes output + +# Network configuration +[net] +retry = 2 # number of times a network call will automatically retried ``` # Environment Variables diff --git a/tests/support/mod.rs b/tests/support/mod.rs index 3ba8be881..112ae6bdb 100644 --- a/tests/support/mod.rs +++ b/tests/support/mod.rs @@ -662,6 +662,7 @@ fn substitute_macros(input: &str) -> String { ("[RUNNING]", " Running"), ("[COMPILING]", " Compiling"), ("[ERROR]", "error:"), + ("[WARNING]", "warning:"), ("[DOCUMENTING]", " Documenting"), ("[FRESH]", " Fresh"), ("[UPDATING]", " Updating"), diff --git a/tests/test_cargo_build_auth.rs b/tests/test_cargo_build_auth.rs index 0baa0b86d..c12bb81a7 100644 --- a/tests/test_cargo_build_auth.rs +++ b/tests/test_cargo_build_auth.rs @@ -72,6 +72,7 @@ test!(http_auth_offered { println!("password=bar"); } "#); + assert_that(script.cargo_process("build").arg("-v"), execs().with_status(0)); let script = script.bin("script"); @@ -91,7 +92,11 @@ test!(http_auth_offered { [dependencies.bar] git = "http://127.0.0.1:{}/foo/bar" "#, addr.port())) - .file("src/main.rs", ""); + .file("src/main.rs", "") + .file(".cargo/config","\ + [net] + retry = 0 + "); assert_that(p.cargo_process("build"), execs().with_status(101).with_stdout(&format!("\ @@ -134,7 +139,11 @@ test!(https_something_happens { [dependencies.bar] git = "https://127.0.0.1:{}/foo/bar" "#, addr.port())) - .file("src/main.rs", ""); + .file("src/main.rs", "") + .file(".cargo/config","\ + [net] + retry = 0 + "); assert_that(p.cargo_process("build").arg("-v"), execs().with_status(101).with_stdout(&format!("\ diff --git a/tests/test_cargo_net_config.rs b/tests/test_cargo_net_config.rs new file mode 100644 index 000000000..b1b22f7cb --- /dev/null +++ b/tests/test_cargo_net_config.rs @@ -0,0 +1,53 @@ +use support::{project, execs}; +use hamcrest::assert_that; + +fn setup() {} + +test!(net_retry_loads_from_config { + let p = project("foo") + .file("Cargo.toml", &format!(r#" + [project] + name = "foo" + version = "0.0.1" + authors = [] + + [dependencies.bar] + git = "https://127.0.0.1:11/foo/bar" + "#)) + .file("src/main.rs", "").file(".cargo/config", r#" + [net] + retry=1 + [http] + timeout=1 + "#); + + assert_that(p.cargo_process("build").arg("-v"), + execs().with_status(101) + .with_stderr_contains(&format!("[WARNING] spurious network error \ +(1 tries remaining): [2/-1] [..]"))); +}); + +test!(net_retry_git_outputs_warning{ + let p = project("foo") + .file("Cargo.toml", &format!(r#" + [project] + name = "foo" + version = "0.0.1" + authors = [] + + [dependencies.bar] + git = "https://127.0.0.1:11/foo/bar" + "#)) + .file(".cargo/config", r#" + [http] + timeout=1 + "#) + .file("src/main.rs", ""); + + assert_that(p.cargo_process("build").arg("-v").arg("-j").arg("1"), + execs().with_status(101) + .with_stderr_contains(&format!("[WARNING] spurious network error \ +(2 tries remaining): [2/-1] [..]")) + .with_stderr_contains(&format!("\ +[WARNING] spurious network error (1 tries remaining): [2/-1] [..]"))); +}); diff --git a/tests/tests.rs b/tests/tests.rs index c41cb9b3c..abfa20382 100644 --- a/tests/tests.rs +++ b/tests/tests.rs @@ -75,6 +75,7 @@ mod test_cargo_version; mod test_shell; mod test_cargo_death; mod test_cargo_cfg; +mod test_cargo_net_config; thread_local!(static RUSTC: Rustc = Rustc::new("rustc").unwrap());